Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convert TokenModel to an ES6 class and extract utils function for calculating lifetime #223

Merged

Conversation

menewman
Copy link
Contributor

Summary

I saw open issue #212 for converting ES5-style classes to modern class syntax, and it seemed like a nice issue for new contributors to help chip away at (small refactoring that can be covered by unit tests; no functional changes).

So, I am submitting a PR to do that for the TokenModel class. Hopefully it is helpful! I'm happy to make changes to better match the desired styles/conventions of the repo as necessary. Or, if this PR isn't in line with what you had in mind, I won't be offended if you'd prefer to close it.

Overview:

  • Converted TokenModel to an ES6-style class (previously it was an ES5-style "constructor function").
  • Extracted a utility function for getting lifetime in seconds from expiration date into a date-util module.
  • Updated unit tests to cover the affected code.

Linked issue(s)

One checkbox of #212 (but not the whole thing).

Involved parts of the project

Specifically the TokenModel, which is used as a representation for access/refresh tokens.

Added tests?

Although I did not make functional changes, I did add a couple of new test cases to make absolutely sure there weren't regressions.

  • New test cases in token-model__test.js to make sure that the model constructor is continuing to work as expected, especially when required arguments are missing, or when custom attributes are passed in.
  • New test file (date-util__test.js) to specifically cover the extracted utility function.

OAuth2 standard

N/A

Reproduction

N/A

@jankapunkt jankapunkt merged commit 74f07c3 into node-oauth:development Aug 17, 2023
4 checks passed
@jankapunkt
Copy link
Member

Thanks a lot @menewman ! If you intend to do more classes, please be careful with changing the tests as I currently work on #219 and update nearly all integration tests. If you keep the changes in the unit tests then it should be okay.

@menewman menewman deleted the fix-convert-token-model-to-es6 branch August 20, 2023 05:45
@jankapunkt jankapunkt mentioned this pull request Aug 29, 2023
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants